Devp2p DPT Type Improvements #1029
Conversation
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
… on malformed ETH/64 status msgs (triggered in client runs)
…ng in client (fixes wrongly propagated timeout error on bootnodes)
|
Ok, open for review, commit messages should be relatively self-descriptive. 😄 |
| if (Buffer.isBuffer(obj)) return [obj.toString('hex')] | ||
| if (typeof obj === 'string') return [obj] | ||
|
|
||
| const keys = [] | ||
| if (Buffer.isBuffer(obj.id)) keys.push(obj.id.toString('hex')) | ||
| if (obj.address && obj.port) keys.push(`${obj.address}:${obj.port}`) | ||
| //if (obj.address && obj.port) keys.push(`${obj.address}:${obj.port}`) |
There was a problem hiding this comment.
should this remain commented out?
There was a problem hiding this comment.
Yes, there is just no property port on peer objects - just updPort and tcpPort, so the line in its current form has no effect and I couldn't decide on selecting for one of the other port properties, in fact I am not understanding well enough what this line is doing TBH. So I left to indicate that there was something which might have had some intention. 😄 Could have left a comment though.
Will also leave to not revoke the approval but rather merge here now.
| try { | ||
| await Promise.all(promises) | ||
| } catch (e) { | ||
| this.error(e) |
There was a problem hiding this comment.
codecov is warning that this line isn't covered, it's not so important, but could add a quick test forcing a fast timeout to ensure the error is caught and propagated properly
There was a problem hiding this comment.
I will leave this for now but generally that's a good idea - also left a comment on the codecov/patch discussion in the chat. Will likely be good if we get to some work mode where we take coverage more serious again (I at least started in #1028 to add some substantial tests and had a look at codecov/patch, took me quite some time but I think I finally had some mental breakthrough in my testdouble understanding 😄 ).
There was a problem hiding this comment.
I've directly taken the occasion and made the codecov/project check mandatory again, together with the other library's test-* checks (e.g. test-trie), think we still had some gap here due to uncertainties in the CI setup for some time (these "run everything or run selected" discussions). And we can of course later always adopt again if some CI setup change comes up.
No description provided.